test/testMinsky.cc (4)

26-28: Consider adding explicit includes for threading facilities.

The test at line 1405 uses this_thread::sleep_for but <thread> and <chrono> headers are not explicitly included. While these may be transitively included, explicit includes would be more robust.

 #include <fstream>
 #include <cstdio>
 #include <algorithm>
+#include <thread>
+#include <chrono>

---

1410-1429: Strengthen assertions in history test.

Line 1418's assertion EXPECT_TRUE(pushed || history.size() > 0) is too weak and may pass even when pushHistory() fails to work correctly. The test should verify the expected behavior more precisely.

Consider testing the actual expected behavior:

       auto var1 = model->addItem(VariablePtr(VariableType::flow, "histVar"));
+      size_t initialSize = history.size();
       bool pushed = pushHistory();
       
-      // Should push only if different from previous
-      EXPECT_TRUE(pushed || history.size() > 0);
+      // Should push if different from previous state
+      if (pushed) {
+        EXPECT_GT(history.size(), initialSize);
+      }

---

1761-1771: Address exportSchema test workaround.

Line 1765 hardcodes schema version 3 with a comment indicating that the default schema 1 crashes. This suggests the test is working around a bug rather than properly testing the functionality.

Consider:

1. If schema version 1 truly crashes, this should be investigated and fixed as a separate issue
2. The test should verify both default and non-default schema versions work correctly
3. If certain schema versions are deprecated or unsupported, document this explicitly

Would you like me to help investigate why schema version 1 crashes and create an issue to track it?

---

1753-1758: Clarify assertion logic.

Line 1757's assertion EXPECT_FALSE(result || reset_flag()) is difficult to understand at a glance. The test appears to expect both result and reset_flag() to be false after calling resetIfFlagged().

Consider splitting into two clear assertions:

       flags |= reset_needed;
       bool result = resetIfFlagged();
-      EXPECT_FALSE(result || reset_flag());
+      EXPECT_FALSE(result); // resetIfFlagged should return false after processing
+      EXPECT_FALSE(reset_flag()); // reset_needed flag should be cleared

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: highperformancecoder
Repo: highperformancecoder/minsky PR: 566
File: test/testCSVParser.cc:154-156
Timestamp: 2025-10-03T23:24:29.823Z
Learning: The Minsky project's test suite is built with C++20 (`-std=c++20` in test/Makefile), so C++20 features like `std::string::starts_with`, `std::format`, ranges, concepts, etc., can be used in test code without compatibility concerns.

Learnt from: highperformancecoder
Repo: highperformancecoder/minsky PR: 566
File: test/testCSVParser.cc:154-156
Timestamp: 2025-10-03T23:24:29.823Z
Learning: The Minsky project's test suite is built with C++20 (`-std=c++20` in test/Makefile), so C++20 features like `std::string::starts_with`, `std::format`, ranges, concepts, etc., can be used in test code without compatibility concerns.

Learnt from: highperformancecoder
Repo: highperformancecoder/minsky PR: 538
File: engine/variableValue.cc:331-351
Timestamp: 2024-12-30T09:34:38.295Z
Learning: The user prefers failing silently for dimension lookups in `variableValue.cc` rather than throwing an exception if a dimension is not found.

test/testMinsky.cc (1)

1693-1717: makeVariablesConsistent test doesn’t yet verify contents/duplicates

This test currently checks only that flowVars() and stockVars() grow to size 2:

EXPECT_GT(godley->flowVars().size(), flowVarsBefore);
EXPECT_GT(godley->stockVars().size(), stockVarsBefore);
EXPECT_EQ(2, godley->flowVars().size());
EXPECT_EQ(2, godley->stockVars().size());

Earlier review feedback from the maintainer asked to verify that specific names are present (e.g. flow1 in flowVars, stock1 in stockVars) and that duplicate entries in the variableValues table are removed. This test still doesn’t check either of those behaviours. Based on that feedback, you might extend it along the lines of:

auto flowNames  = godley->flowVars();
auto stockNames = godley->stockVars();

auto hasFlow1  = std::any_of(flowNames.begin(),  flowNames.end(),  [](auto& v){ return v->name()=="flow1"; });
auto hasStock1 = std::any_of(stockNames.begin(), stockNames.end(), [](auto& v){ return v->name()=="stock1"; });

EXPECT_TRUE(hasFlow1);
EXPECT_TRUE(hasStock1);
// Optionally: assert there are no duplicate entries here or in variableValues

and add a separate setup that deliberately creates duplicate variableValues entries, then confirms they are de‑duplicated after makeVariablesConsistent().

test/testMinsky.cc (8)

1318-1327: Avoid depending on multipleEquities(bool) return semantics

The test currently assumes multipleEquities(true) returns true:

EXPECT_FALSE(multipleEquities());
bool result = multipleEquities(true);
EXPECT_TRUE(result);
EXPECT_TRUE(multipleEquities());

If the setter returns the previous value (a common pattern), this will fail even though behaviour is correct. You can make the test robust by only asserting on the getter:

-      EXPECT_FALSE(multipleEquities());
-      bool result = multipleEquities(true);
-      EXPECT_TRUE(result);
-      EXPECT_TRUE(multipleEquities());
+      EXPECT_FALSE(multipleEquities());
+      multipleEquities(true);        // ignore return value
+      EXPECT_TRUE(multipleEquities());

The rest of the test (toggling back to false) can remain as‑is.

---

1409-1428: History test could assert more specific expectations

historyOperations uses a fairly loose condition:

bool pushed = pushHistory();
// Should push only if different from previous
EXPECT_TRUE(pushed || history.size() > 0);

After clearHistory(), if pushHistory() is expected to always push once, you can tighten this to catch more regressions, for example:

-      bool pushed = pushHistory();
-      // Should push only if different from previous
-      EXPECT_TRUE(pushed || history.size() > 0);
+      bool pushed = pushHistory();
+      // First push after clearHistory() should add an entry
+      EXPECT_TRUE(pushed);
+      EXPECT_GT(history.size(), 0u);

Not critical, but worth considering to make the test more meaningful.

---

1536-1571: Autosave timing: consider polling instead of a single fixed sleep

The autosave test relies on a single sleep_for(100ms) before checking exists(testFile). On slower machines or under load, autosave might complete later and cause intermittent failures.

You could keep the same overall delay but poll with a timeout, e.g.:

-      // Give some time for autosave to complete (it runs in background)
-      std::this_thread::sleep_for(std::chrono::milliseconds(100));
-
-      // Verify the autosave file was created
-      EXPECT_TRUE(exists(testFile));
+      // Give some time for autosave to complete (it runs in background)
+      using namespace std::chrono_literals;
+      bool found = false;
+      for (int i = 0; i < 10 && !found; ++i)
+      {
+        if (exists(testFile)) found = true;
+        else std::this_thread::sleep_for(10ms);
+      }
+      EXPECT_TRUE(found);

This keeps the test deterministic while reducing flakiness risk.

---

1668-1688: Comment and expectations for insertGroupFromFile are inconsistent

Here the comment says “model->items should be empty”, but the assertion expects exactly one item:

// model->items should be empty (time operation is removed by clearAllMaps and restored by load)
EXPECT_EQ(1, model->items.size()); // Only time operation

This is self‑contradictory and could confuse future readers. Consider aligning the comment with the assertion:

-      // model->items should be empty (time operation is removed by clearAllMaps and restored by load)
-      EXPECT_EQ(1, model->items.size()); // Only time operation
+      // After insertGroupFromFile, only the implicit time operation should be in model->items
+      EXPECT_EQ(1, model->items.size());

The rest of the test (groups and nested items) looks good.

---

1720-1750: Questionable assumptions in garbageCollect test setup

This test expects constructEquations() to populate flowVars, stockVars, equations, and integrals:

auto var1 = model->addItem(VariablePtr(VariableType::flow, "gcVar"));
auto op1  = model->addItem(OperationPtr(OperationType::add));
model->addWire(op1->ports(0), var1->ports(1));

constructEquations();
EXPECT_GT(flowVars.size(), 0);
EXPECT_GT(stockVars.size(), 0);
EXPECT_GT(equations.size(), 0);
EXPECT_GT(integrals.size(), 0);

With only a single add operation and no IntOp/GodleyIcon, it’s not obvious that integrals (or even stockVars) will be non‑empty; this may make the test brittle or outright failing, depending on how constructEquations() is defined.

You might want to either:

• Build a slightly richer model that’s guaranteed to yield non‑empty flowVars/stockVars/equations/integrals (e.g. including an IntOp or a small Godley example), or
• Relax the pre‑garbageCollect() expectations and focus this test purely on “whatever is there gets cleared”.

Please double‑check the current behaviour of constructEquations() against these assertions to ensure the test isn’t encoding an incorrect assumption.

---

1838-1842: Confirm resetIfFlagged return convention

The test asserts:

flags |= reset_needed;
bool result = resetIfFlagged();
EXPECT_FALSE(result || reset_flag());

which is equivalent to requiring:

• result == false, and
• reset_flag() == false (i.e., flag cleared).

That matches a convention “return false if a reset happened”, but many APIs use the opposite (“return true if a reset was performed”). Please confirm this matches the actual implementation and intended contract; if not, consider flipping the expectation to e.g.:

EXPECT_TRUE(result);
EXPECT_FALSE(reset_flag());

so callers can interpret the result more intuitively.

---

1845-1855: Use portable, unique temp path in exportSchema test

exportSchema still hard‑codes a /tmp path:

string schemaFile = "/tmp/test_schema.xsd";
exportSchema(schemaFile,3);
...
ifstream f(schemaFile);
...
remove(schemaFile.c_str());

This is non‑portable and can collide with concurrent test runs. You’re already using Boost.Filesystem elsewhere; I’d suggest:

-      string schemaFile = "/tmp/test_schema.xsd";
+      auto schemaPath = temp_directory_path() / unique_path("test_schema_%%%%-%%%%.xsd");
+      std::string schemaFile = schemaPath.string();

The rest of the test can continue to use schemaFile as a string.

---

1638-1644: srandTest provides minimal value

This test only calls srand twice and asserts nothing beyond “no crash”:

// Just verify it doesn't crash
srand(42);
srand(123);

Given srand is a trivial C library function, this doesn’t really exercise Minsky code and may be considered noise in the suite (similar to other “useless tests” that were previously removed). I’d recommend removing it, or, if you want to keep it, at least verify some observable behaviour that depends on the seed.